GitHub Action for non-UTF-8 locales - #7821
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7821 +/- ##
=======================================
Coverage 99.01% 99.01%
=======================================
Files 88 88
Lines 17234 17234
=======================================
Hits 17065 17065
Misses 169 169 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
MichaelChirico
left a comment
There was a problem hiding this comment.
I think it could be merged as-is, PTAL at the feedback and see what you agree is worth addressing. Thanks!
|
BTW, in #7832 ( |
|
Here I'm running tests with only (Try installing function(locale, strings) {
old_collate = Sys.getlocale('LC_COLLATE')
Sys.setlocale('LC_COLLATE', locale)
on.exit(Sys.setlocale('LC_COLLATE', old_collate), add = TRUE)
old_ctype = Sys.getlocale('LC_CTYPE')
Sys.setlocale('LC_CTYPE', locale)
on.exit(Sys.setlocale('LC_CTYPE', old_ctype), add = TRUE)
data.table::data.table(strings, locale = base::order(strings), forderv = data.table:::forderv(strings))
}) |
|
Yea, I'm wondering if we should combine the efforts here with the |
|
It's probably fine to move the encoding tests into the |
060f490 to
e5b3b62
Compare
|
OK, moved the changes there. The behemoth grows :) |
f7e51f8 to
3b48dd9
Compare
3b48dd9 to
2a36892
Compare
| - os: macOS-latest | ||
| locale: 'zh_CN.utf8' | ||
| - os: macOS-latest | ||
| locale: 'fr_CA.ISO-8859-1' |
There was a problem hiding this comment.
wont we have to use 'fr_CA' in the exclusion as well? same for the exclusion of windows
ben-schwen
left a comment
There was a problem hiding this comment.
LGTM besides the potential clash of 'fr_CA.ISO-8859-1' and 'fr_CA'
| # Multibyte characters: Mandarin | ||
| 'zh_CN.utf8', | ||
| # Encoding: non-UTF-8 locales for French, Mandarin, and Russian | ||
| 'fr_CA', 'zh_CN.GB18030', 'ru_RU.KOI8-R', # fr_CA is implicitly 'ISO-8859-1' |
There was a problem hiding this comment.
It's not completely reliable to rely on the implicit default encoding. It's possible to have a UTF-8 fr_CA locale with the right settings in /etc/locale.gen:
echo 'fr_CA UTF-8' | sudo tee -a /etc/locale.gen
sudo locale-gen --keep-existing
LANG=fr_CA Rscript -e 'l10n_info()'$MBCS
[1] TRUE
$`UTF-8`
[1] TRUE
$`Latin-1`
[1] FALSE
$codeset
[1] "UTF-8"
| sudo tee "$target" << EOF | ||
| #!/bin/bash | ||
| set -o pipefail | ||
| exec "${target}.orig" "\$@" 2>&1 | iconv -c -t UTF-8 |
There was a problem hiding this comment.
This will cause child R processes run by R CMD check to encode their output to UTF-8 as well. Then the overall output from the parent process will be encoded once again, with some outputs requiring echo "$unicode_text_from_browser" | iconv -t $source_encoding | iconv -t $source_encoding to read them as UTF-8 (implying some encoding from $source_encoding to UTF-8 has happened thrice, some even more).
There was a problem hiding this comment.
Yea, I got as far as that, but not fixing it.
In ru_RU.KOI8-R, it looks like the root issue is {bit} not installing, possibly because of UTF-8 in R/ comments like:
https://github.com/r-lib/bit/blob/d128f0735f3c15ac67070a1398fea3411de5eab8/R/zzz.R#L2
This per LLM:
The Recursive Mojibake (Why the text is so garbled)
- Child Process: Encounters an error and correctly outputs Russian text in
KOI8-R(e.g.Предупреждение). Itsiconvwrapper converts this toUTF-8. - Parent Process (
R CMD INSTALL): Captures thatUTF-8text and echoes it to the console. The parent'siconvwrapper intercepts theseUTF-8bytes, incorrectly assumes they areKOI8-R, and converts them toUTF-8again. - Grandparent Process (
R CMD check): Captures the double-mojibake, assumes it isKOI8-R, and converts it a third time.
If we reverse the triple-encoding of the string in your log (п©ц╥я▐Б■─...), it translates perfectly to the standard R error:
"Error in parse(con, keep.source = FALSE, srcfile = NULL) : invalid input found on input connection..."
| } | ||
| } | ||
| shell: Rscript {0} | ||
| shell: bash -c 'set -o pipefail; Rscript {0} 2>&1 | iconv -c -t UTF-8' |
There was a problem hiding this comment.
{0} probably included quotes or something. You can safely pass arbitrary arguments to shell commands like this:
| shell: bash -c 'set -o pipefail; Rscript {0} 2>&1 | iconv -c -t UTF-8' | |
| shell: bash -c 'set -o pipefail; Rscript "$@" 2>&1 | iconv -c -t UTF-8' -- {0} |
There was a problem hiding this comment.
This is so silly. What GHA runner actually runs is not the command line specified in the shell argument, oh no. It splits the shell line by whitespace and makes every word into a separate argument. As a result, the command being run is
bash "-c" "'set" "-o" "pipefail;" "Rscript" '"$@"' "2>&1" "|" "iconv" "-c" "-t" "UTF-8'" "--" "/home/runner/work/_temp/21fe02f1-a9e0-4138-ba8c-875e7bb7469a.sh"which gives the same error:
-o: -c: line 1: unexpected EOF while looking for matching `''
Naturally, this is not documented at all.
There was a problem hiding this comment.
That approach is kinda doomed because this is shared with windows runners too, right?
There was a problem hiding this comment.
Quite.
I'm testing a solution in 1e27ce0 (https://github.com/Rdatatable/data.table/actions/runs/31110427909), limiting the time spent running with a non-default locale to R CMD check only.
Following #7681 (comment): test
data.tablein the Latin-1 locale (whereCE_NATIVEstrings should be byte-to-byte equal toCE_LATIN1), GB18030 (which is fully Unicode-compatible, but the mapping from code points to byte sequences is very non-uniform), KOI8-R (which can represent some math symbols but not extended Latin or CJK).